I think we could have a much simpler and effective setup here. If we want to know whether running collect stops the world, it's better to spawn a thread that keeps calling reader.collect on a loop. And have other threads, record measurements simultaneously.

I did that initially. The problem was that collection phase was not realistic as it had 0 measurements and basically held lock in a loop. So I decided to make it more realistic, by generating some measurements so collection wouldn't be empty.

The problem was that collection phase was not realistic as it had 0 measurements and basically held lock in a loop

I don't follow. You could always record some measurements before you call collect. My concern is around the way this test is setup. The test is using some custom "iterations" which seems unnecessary. You could simply do this instead:

1. Emit some measurements.
2. Spawn a thread that runs collect in a loop.
3. Spawn additional threads that record measurements.
4. Calculate throughput for the measurements recorded in step 3.

If doing the above, leads to zero measurements being recorded, then we have our answer: Collect is going to "stop the world" as long as it runs.

As I mentioned in this comment, unless we plan to use a more efficient synchronization mechanism in ValueMap, this stress test would not be adding any value.

First of all, this test is adding value, as it is able to measure difference with different "collect" implementation.
Second, running collect in the loop doesn't work with delta temporality, because after first run there will be zero measurements and all other iterations will basically hold write lock in the loop. While in reality no one runs collect in loop and usually there are many measurements and its important that write lock is held as short as possible (which is not the case with current implementation, hence I was able to improve it and this test actually measures it).
I could probably have different tests for delta an cumulative temporality, but I decided to emulate realistic environment.

While in reality no one runs collect in loop

In reality, no one runs the collect the way it's run in this stress test either. Collect would usually be run periodically in 10/30/60 seconds intervals.

First of all, this test is adding value, as it is able to measure difference with different "collect" implementation.

usually there are many measurements and its important that write lock is held as short as possible (which is not the case with current implementation, hence I was able to improve it and this test actually measures it).

This test claims to measure update throughput "while collect is running". The actual implementation of this test however relies on squeezing in some updates before collect runs. That's not the same as testing "while collect is running".

I would love to improve collect to take the write lock for the shortest possible time, but it's better tested using a benchmark.

The actual implementation of this test however relies on squeezing in some updates before collect runs.

There's a lot of truth to this statement, but none the less it appears to measure something useful :)

Ideally I would like to "catch" changes to how attribute-set hashmap are locked with both: existing types of measurements (with existing attribute-set and new attribute-set combination).
Maybe we need to tests these things separately... I don't know, any ideas are welcome :)

This can be done without spawning new threads. (before you spawn threads for running collect and recording measurements)

In the PR description, you mentioned that you saw a major improvement when running this with the changes for #2145 locally. I'm curious, the code changes for #2145 are mostly related to reuse of collect code across instruments. How is that improving the perf numbers?

Throughput perf here should mostly depend on the thread synchronization mechanism used for collect and update. Currently, it's using a RwLock in ValueMap and the collect operation requires a write lock which would "stop the world" until collect has the write lock. Unless you change that, I wouldn't expect any significant improvement in throughput here.

For delta temporality I was able to reduce the amount of time a write lock is held while collecting.
For cumulative temporality I simply changed to read lock (currently there is write lock, although cumulative temporality doesn't modify anything, and IMO locking individual measurements is sufficient and much better, then "stop the world until all measurements are read" approach). Although read lock is better, but measurements with new attribute-sets will still be locked. I see two more alternatives:

• clone entire attribute-set hashmap (acquire read lock, clone hashmap, release lock, iterate individual measurements)
• implement some sort of sharding (more throughtput at the cost of single measurement performance)

In any case, there are ways to improve it:) so I created this PR so we could actually measure different ideas.

These are good ideas!

For cumulative temporality I simply changed to read lock (currently there is write lock, although cumulative temporality doesn't modify anything,

Great! In that case, we should be able test update throughput while collect runs in a loop for Cumulative and a use a simple benchmark for Delta. 🙂

implement some sort of sharding (more throughtput at the cost of single measurement performance)

I did try sharding approach earlier in #1564 - to reduce the write-lock duration by locking only the specific shard of measurements during collection. However, I encountered some concurrency issues. It’s still an approach worth revisiting at some point.

Great! In that case, we should be able test update throughput while collect runs in a loop for Cumulative and a use a simple benchmark for Delta. 🙂

+1